feat(cli): add --staged flag #2300

castarco · 2024-04-04T10:25:43Z

Summary

This PR introduces a new --staged flag that allows biome to select only the files that are staged to be committed.

Motivation

Long version

Fixes 📎 Feature: --staged option to simplify the creation of git hook scripts #2296
Related Discord discussion: https://discord.com/channels/1132231889290285117/1222450681919832134

TLDR

There is some semblance with --changed, but its behaviour is different:

--changed is useful to run certain checks in CI environment, selecting only the files that changed between two git "refs" (they can be explicit or implicit).
In contrast, --staged is useful in local environments, to be used in git hooks such as pre-commit.

Test Plan

I introduced new unit tests covering basic behaviour changes
I tested the generated binary manually to verify that it works exactly as expected

"Mixed" Performance Results

What follows is for a very small repository.

When there are no changes

Performance is slightly better

When there is a small number of changes

Performance is slightly worse 🤔, I suspect that this because of the extra git call.

I hypothesize that this extra overhead should be "absorbed" in bigger repositories with many more files.

netlify · 2024-04-04T10:25:58Z

✅ Deploy Preview for biomejs ready!

Name	Link
🔨 Latest commit	`40d812b`
🔍 Latest deploy log	https://app.netlify.com/sites/biomejs/deploys/6618ee6ad4c97f0008823723
😎 Deploy Preview	https://deploy-preview-2300--biomejs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 4 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 93 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

ematipico

I had a quick look, and it seems that the CI command isn't covered?

I know that the new option shouldn't work in CI, but we should at least terminate the process and raise a diagnostic, saying that this option isn't valid in the CI environments

ematipico

I only noticed now that the new options were added only for specific commands, so you can discard my previous comment. I left few suggestions, and I think we can merge the PR once they are addressed

ematipico · 2024-04-11T07:32:53Z

crates/biome_cli/src/commands/format.rs

+    if since.is_some() {
+        if !changed {
+            return Err(CliDiagnostic::incompatible_arguments("since", "changed"));
+        }
+        if staged {
+            return Err(CliDiagnostic::incompatible_arguments("since", "staged"));
+        }
    }

    if changed {
+        if staged {
+            return Err(CliDiagnostic::incompatible_arguments("changed", "staged"));
+        }
        paths = get_changed_files(&session.app.fs, &configuration, since)?;
+    } else if staged {
+        paths = get_staged_files(&session.app.fs)?;
    }


Considering how the code is repeated, maybe it makes more sense to have a function in commands/mod.rs that does the check, and each command imports it and calls it.

Extracted 🏗️ .

CHANGELOG.md

website/src/content/docs/internals/changelog.md

Signed-off-by: Andres Correa Casablanca <[email protected]>

return error when --since and --staged are set at the same time. Signed-off-by: Andres Correa Casablanca <[email protected]>

Signed-off-by: Andres Correa Casablanca <[email protected]>

ematipico

This is a great addition; thank you!

Conaclos · 2024-04-12T10:29:09Z

While I was writing the blog post for the Biome 1.7, I wonder if we should not rename --staged to --staged-files to highlight the fact that unstated changes in staged files are not ignored.

Any opinion?

ematipico · 2024-04-12T10:49:22Z

highlight the fact that unstated changes in staged files are not ignored.

I don't understand what you mean. I think --staged is fine; it's the git dialect.

Conaclos · 2024-04-12T11:29:55Z

I don't understand what you mean. I think --staged is fine; it's the git dialect.

i mean that you can stage a file and then modify it:

❯ git status
On branch main
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   a

❯ echo "more" >> a

❯ git status
On branch main
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   a

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   a

You end with a staged file with unstaged changes.

castarco · 2024-04-12T12:55:32Z

@Conaclos Regarding your first point, you are right. Although I think it will be difficult to describe, and also to find "good solutions".

I'll try to expand what I understand you said:

Let's say I have 3 staged files: A, B, and C.
Let's also say that C has new changes after it has been staged.
When biome format/check/lint reads the file C it will see what we have in our local filesystem (including the changes after it has been staged), and it will apply its fixes over that last version, not over what was staged.

For this I imagine 2 potential solutions:

When there is a discrepancy between "staged" and "local filesystem", Biome could issue a warning.
- This one requires an extra git command to obtain the list of unstaged changes.
A more sophisticated and aggressive approach would involve running a pair of git stash and git stash pop commands (before and after the fixes).
- This one requires 2 extra git commands (and they are also slower because they imply filesystem writes)

You end with a staged file with unstaged changes.

That's true, although I think it's a different problem from the previous one, and there are some relevant points that make it easier to understand and solve:

many people run their hooks as linters without --apply.
Without staged, when we perform the git command to obtain the staged files, many times "we" filter them by extension before passing them to Biome. This can be a problem because, in a way, it is equivalent to having multiple sources of truth (we have "include/exclude" filters in Biome's configuration, but also in our shell script)
This functionality can be further extended in the future, in two separate ways (not necessarily exclusive, but if any of them is implemented, then the other becomes irrelevant):
- Making it possible for the biome format and biome check commands to output only the list of files they changed. This could be passed directly as arguments to git add.
  
  I imagine something like git add $(biome check --staged --apply --list-fixed .) (I didn't check yet if such an option already exists)
- Internalizing the git add command inside Biome (although I prefer the previous option over this one, as it is lighter and more generic)

I'd be happy to discuss what you and other people think about these options, and if there are any other ideas floating around related to this.

Conaclos · 2024-04-12T14:21:48Z

@castarco
I didn't suggest fixing it, just renaming the flag to be clear about that.
Any way I am also happy with --staged. I was just expressing a concern/idea.

For this I imagine 2 potential solutions:

When there is a discrepancy between "staged" and "local filesystem", Biome could issue a warning.
This one requires an extra git command to obtain the list of unstaged changes.
A more sophisticated and aggressive approach would involve running a pair of git stash and git stash pop commands (before and after the fixes).
This one requires 2 extra git commands (and they are also slower because they imply filesystem writes)

The first option might be nice :)
This seems to be a good fit for Biome.

I think the second suggestion is a bad idea, because if the user uses --apply/--write, you can end up with conflicts when reapplying the stashed changes.
This can be very confusing for users.
This sort of thing should be left to dedicated tools like git-format-staged and the like.

Without staged, when we perform the git command to obtain the staged files, many times "we" filter them by extension before passing them to Biome. This can be a problem because, in a way, it is equivalent to having multiple sources of truth (we have "include/exclude" filters in Biome's configuration, but also in our shell script)

I am unsure to follow you there. Users should rely only on a Biome configuration and --files-ignore-unknown=true to filter files.

Making it possible for the biome format and biome check commands to output only the list of files they changed. This could be passed directly as arguments to git add.

I am not sure if there is so much value for that. A user could simply run git update-index --again — assuming that there is no unstaged changes before the formatting/linting.
However, I am not opposed to the idea, it could have other applications...

Internalizing the git add command inside Biome (although I prefer the previous option over this one, as it is lighter and more generic)

I think that it is a bad idea because this could be confusing and blur the line between Biome and Git.

github-actions bot added A-CLI Area: CLI A-Core Area: core labels Apr 4, 2024

github-actions bot added the A-Changelog Area: changelog label Apr 4, 2024

castarco marked this pull request as ready for review April 4, 2024 10:35

castarco force-pushed the feat-staged-flag branch from 8e43888 to 7d7af88 Compare April 4, 2024 10:40

github-actions bot added the A-Website Area: website label Apr 4, 2024

castarco changed the title ~~feat: add --staged flag~~ feat(cli): add --staged flag Apr 4, 2024

castarco mentioned this pull request Apr 5, 2024

🐛 --changed in monorepos throws No such file or directory (os error 2) #1996

Closed

1 task

castarco force-pushed the feat-staged-flag branch 2 times, most recently from 31a28a9 to c5c773b Compare April 5, 2024 10:06

ematipico reviewed Apr 10, 2024

View reviewed changes

ematipico approved these changes Apr 11, 2024

View reviewed changes

castarco force-pushed the feat-staged-flag branch from 4d5f96f to a0e859b Compare April 11, 2024 16:40

castarco added 9 commits April 12, 2024 10:13

feat: add --staged flag to check,format,lint

6872253

Signed-off-by: Andres Correa Casablanca <[email protected]>

test: --staged flag tests

c34e9c7

Signed-off-by: Andres Correa Casablanca <[email protected]>

chore: apply just f

23ce9cc

Signed-off-by: Andres Correa Casablanca <[email protected]>

docs: add new --staged to CHANGELOG

f003e8c

Signed-off-by: Andres Correa Casablanca <[email protected]>

fix: cli opts incompatibility

41b86b8

return error when --since and --staged are set at the same time. Signed-off-by: Andres Correa Casablanca <[email protected]>

perf: avoid conditional in one execution path

671ac23

Signed-off-by: Andres Correa Casablanca <[email protected]>

fix: skip files outside current dir when using --staged

6264a2f

Signed-off-by: Andres Correa Casablanca <[email protected]>

docs: fix typo

b82df4e

Signed-off-by: Andres Correa Casablanca <[email protected]>

refactor: extract function

40d812b

Signed-off-by: Andres Correa Casablanca <[email protected]>

castarco force-pushed the feat-staged-flag branch from a0e859b to 40d812b Compare April 12, 2024 08:18

ematipico approved these changes Apr 12, 2024

View reviewed changes

ematipico merged commit af121e3 into biomejs:main Apr 12, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add --staged flag #2300

feat(cli): add --staged flag #2300

castarco commented Apr 4, 2024 •

edited

Loading

netlify bot commented Apr 4, 2024 •

edited

Loading

ematipico left a comment

ematipico left a comment

ematipico Apr 11, 2024

castarco Apr 11, 2024

ematipico left a comment

Conaclos commented Apr 12, 2024

ematipico commented Apr 12, 2024

Conaclos commented Apr 12, 2024

castarco commented Apr 12, 2024 •

edited

Loading

Conaclos commented Apr 12, 2024

feat(cli): add --staged flag #2300

feat(cli): add --staged flag #2300

Conversation

castarco commented Apr 4, 2024 • edited Loading

Summary

Motivation

Long version

TLDR

Test Plan

"Mixed" Performance Results

When there are no changes

When there is a small number of changes

netlify bot commented Apr 4, 2024 • edited Loading

✅ Deploy Preview for biomejs ready!

ematipico left a comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

ematipico Apr 11, 2024

Choose a reason for hiding this comment

castarco Apr 11, 2024

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Conaclos commented Apr 12, 2024

ematipico commented Apr 12, 2024

Conaclos commented Apr 12, 2024

castarco commented Apr 12, 2024 • edited Loading

Conaclos commented Apr 12, 2024

castarco commented Apr 4, 2024 •

edited

Loading

netlify bot commented Apr 4, 2024 •

edited

Loading

castarco commented Apr 12, 2024 •

edited

Loading